Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

LogOutputStream.create(LineConsumer) #107

Merged
merged 14 commits into from
Mar 15, 2024
Merged

LogOutputStream.create(LineConsumer) #107

merged 14 commits into from
Mar 15, 2024

Conversation

panchenko
Copy link
Contributor

So instead of creating an anonymous class it can be one-liner with lambda expression.

@panchenko
Copy link
Contributor Author

If we are going to proceed with this, then I can also add an example to the readme file.

nemecec
nemecec requested changes Mar 3, 2023
View reviewed changes
Copy link
Collaborator

@nemecec nemecec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool idea, thanks! I think the JavaDoc could be better (I added some comments with suggestions, feel free to use those as inspiration).
Also, a unit test and example usage in README would be much appreciated.

panchenko
panchenko commented Mar 4, 2023
View reviewed changes
README.md Outdated

```java
new ProcessExecutor().command("java", "-version")
.redirectOutput(LogOutputStream.create(line -> ...))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Theoretically, there could be also a new method here .processOutput(line -> ...), WDYT?

@aalmiray
Copy link
Contributor

Bytecode compatibility is set to Java 6 as a minimum which will prevent the use of lambdas.
I'd suggest bumping to 7 as a minimum as JDK 17 (see PR for build) fails if 6 is chosen.
Another option would be to bump to Java 8 as a minimum, in which case lambdas may be used.

Bear in mind that bumping Java baseline constitutes a breaking change and thus the library should increase its major version number according to semver.

@panchenko
Copy link
Contributor Author

The lambdas will be not here, but in code using this library.

@panchenko
Copy link
Contributor Author

@nemecec Could you lease take a look? Thanks!

@panchenko
Copy link
Contributor Author

Is this project maintained?

@nemecec
Copy link
Collaborator

nemecec commented Mar 15, 2024

Is this project maintained?

Sorry, I totally forgot about this PR. Looks good, I'll merge it in.

@nemecec nemecec merged commit 2ba09d1 into zeroturnaround:main Mar 15, 2024
4 checks passed
@nemecec
Copy link
Collaborator

nemecec commented Mar 15, 2024

Thanks for the PR!

# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@nemecec nemecec nemecec requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@panchenko @aalmiray @nemecec